fix(ci): agy reviewer must fall back on GitHub's FILES limit, not just lines - #334
Merged
Conversation
…t lines
Synced from RustyNES, where this was found and fixed (PR #374). The `review`
check fails on a wide PR with `gh pr diff failed:` and an empty reason, which
reads like a lapsed OAuth session on the self-hosted runner. It is neither --
the script has a bug.
GitHub refuses an oversized PR diff two ways, both HTTP 406, with different
wording: over 20,000 LINES, and over 300 FILES. The local-diff fallback was
gated on
grep -qi 'diff exceeded the maximum number of lines'
so only the lines variant reached it. A wide-but-shallow PR -- hundreds of
files, well under the line limit, which is what a bulk regeneration of test
baselines produces -- took the `else` branch and exited 1. That is the one
outcome the fallback exists to prevent; the comment directly above it says "a
large PR is exactly the one worth reviewing".
Verified on RustyNES rather than assumed: fetching the PR head and the base into
the private `refs/agy/*` namespace and diffing against the merge base produced
12,002 lines / 586,010 bytes for a 487-file PR, well inside the 5 MB
`MAX_DIFF_BYTES` cap, so the review proceeds normally once the grep matches.
Includes the two review findings from that PR: the log line now names whichever
limit actually fired (reporting "20,000-line" for a file-count refusal is the
same misleading signal that caused the wrong diagnosis), and the explanatory
comment carries no cross-repo issue number, which would mean nothing here.
Note this cannot take effect on the PR that carries it: the workflow checks out
the DEFAULT BRANCH for the reviewer scripts by design, so a PR cannot rewrite
its own reviewer.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes the Antigravity (agy) reviewer fallback logic so that it properly falls back to a local git diff when GitHub refuses an oversized API diff due to either exceeding the 20,000-line limit or the 300-file limit. This improves reliability and avoids misleading “auth failure” triage signals on wide-but-shallow PRs (common with large baseline regenerations).
Changes:
- Extend the oversized-diff detection to match both “maximum number of lines” and “maximum number of files” GitHub 406 variants.
- Improve logging to report which specific GitHub diff limit was hit (“20,000-line” vs “300-file”) before falling back to local diffing.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…lone
Synced from RustyNES (PR #375), stacked on the file-count fix in this same
branch. That fix alone is not enough: it gets the fallback started, and then the
run dies one step later.
[agy-review] diff exceeds GitHub's 300-file API limit; falling back to a local git diff
[agy-review] fetched PR refs using git's ambient credentials
[agy-review] could not compute the merge base for PR #373
The workflow checks out with `fetch-depth: 1`, deliberately -- the comment says
the fallback "fetches exactly the two refs it needs on demand, so a full history
clone would be paid on every run for a path most runs never take". But two refs
fetched into a shallow repository arrive as DISCONNECTED shallow histories, with
no common ancestor for `git merge-base` to find, so it fails even though both
fetches succeeded.
Asks the compare API for the merge base and fetches that single commit instead
of unshallowing. Diffing two commits needs both trees, not the history between
them, so a shallow fetch of the merge base suffices, and a repo with a large
history does not pay a full clone on a path that exists only because the PR is
already unusually big. `--deepen=250` covers a server that refuses a bare-SHA
fetch.
Verified upstream against a genuinely shallow clone -- the earlier attempt was
checked in a full clone, where `git merge-base` succeeds and the bug cannot be
seen. Reproduced the runner's exact failure at `--depth 1`, then confirmed the
fix in that same clone: merge base resolved via the API (identical to a full
clone's `git merge-base`), SHA fetch accepted, resulting diff 12,021 lines /
586,242 bytes.
Owner
Author
|
Stacked a second commit on this branch: the file-count fix alone is not enough. With it applied, the reviewer gets into the local-diff fallback and then dies one step later, because the workflow checks out with The second commit resolves the merge base through the compare API and fetches that single commit, rather than unshallowing — diffing two commits needs both trees, not the history between them. Verified upstream against a genuinely shallow clone. The first attempt was checked in a full clone, where Merging this branch with only the first commit would leave the reviewer still broken on wide PRs, just failing later. |
Copilot flagged the raw `${base_ref}` interpolation, predicting that a
`<type>/<short-desc>` branch would make the path ambiguous and fail.
That specific case does NOT reproduce -- checked against a real slashed branch,
GitHub's compare endpoint accepts `fix/agy-review-shallow-merge-base...main`
raw and returns the same SHA as the encoded form. So the stated failure is not
the bug.
The finding still points at a real one, for different characters. Git permits
`%` and `#` in a ref name and a URL path does not survive either: `%`
starts an escape sequence, `#` truncates at the fragment. Both would fail
silently into the `|| true` and leave the merge base unresolved, which is
exactly the failure mode this whole fallback exists to avoid.
Encoding with jq's `@uri`. Verified both shapes still resolve: a plain `main`
and a slashed branch encoded to `fix%2F...` both return the correct merge base.
The Antigravity reviewer posted a fresh comment each round and DELETED the previous one. That kept the PR tidy and destroyed the record: a round nobody had read before the next push was gone, with nothing on the PR indicating it had ever existed. Unlike a CodeRabbit or Copilot review thread, an unaddressed finding left no trace -- so a clean comment list was not evidence that nothing had been raised. Observed on RustyNES PR #428. Round 1 posted at 13:37:57Z and round 2 at 14:21:35Z; afterwards the issue-comments endpoint returned exactly ONE bot comment, with created_at equal to updated_at equal to 14:21:35Z. The first was gone -- not edited, since the timestamps would differ, and not appended to. Both rounds raised a blocking issue and both were correct, one of them a data-loss defect, so losing a round is not a hypothetical cost. There is now ONE comment per PR, edited in place: the newest round on top, every earlier round folded into a collapsed <details> block beneath it. Same tidiness, nothing destroyed. The script issues no DELETE at all, and the selftest asserts the absence of one so the behaviour cannot return unnoticed. The archive is bounded by MAX_BODY_BYTES (60000, under GitHub's 65536 hard limit), because a PR with many pushes would otherwise grow it until an EDIT starts failing -- stranding the comment at whatever round last fit, which is the worst failure available since the newest review is the one that cannot post. Oldest rounds drop first, and the drop is ANNOUNCED: a silent truncation would look exactly like a PR reviewed only once, which is the confusion this change exists to remove. Every failure path falls back to a plain post; a duplicate is noise, failing to publish a review is not. THE FORMAT LIVES IN ITS OWN FILE, AND THAT IS THE POINT scripts/_agy_comment_body.sh holds the sentinels and the split/trim helpers and is sourced by both the reviewer and its selftest. agy-review.sh does its work at top level and cannot be sourced, which is exactly how a test ends up reimplementing its subject -- and that happened. The first version of these checks inlined its own copy of the awk pipeline, so a mutation deleting the marker strip came back NOT CAUGHT. A test that reimplements what it tests agrees with itself forever. The fixture changed for the same reason: it had our own bot's comment first, so `first` selected it whether or not the author filter was present, leaving the control that stops any user pasting the marker into a comment and having the bot edit it untestable. A User comment carrying the marker now sorts ahead of ours. Eight mutations, all caught: the author filter, empty-versus-null, oldest-versus- newest selection, a reintroduced DELETE, the marker strip, the archive split's sentinel ordering, dropping the newest round instead of the oldest, and a drop that succeeds on an empty archive (which would spin the trim loop). Verified end to end by simulating four rounds through the real functions: all four findings present in the final body, newest first, marker appearing exactly once. ALSO IN THIS SWEEP actions/checkout is SHA-pinned. This job runs on a SELF-HOSTED runner -- the maintainer's own machine, holding the agy CLI's OAuth session -- so a compromised tag executes there rather than in a disposable VM. Verified rather than copied: tag v7 resolves to 3d3c42e5aac5ba805825da76410c181273ba90b1, the v7.0.1 commit of 2026-07-17. The trailing `# v7` is what Dependabot reads to keep the pin current. .github/actionlint.yaml declares the self-hosted `agy` label. Without it actionlint reported an error on EVERY run in this repo, and a linter that always reports something is a linter that stops being read. Created only when absent, so a repo with its own actionlint config keeps it. _agy_comment_body.sh is REQUIRED, not optional -- agy-review.sh sources it at startup, so an install without it fails at runtime rather than degrading. The installer copies it and the selftest, the workflow chmods it, and both temporaries the archive path creates are pre-declared so the cleanup trap frees them on every exit including the early one after a successful edit. THIS DOES NOT TAKE EFFECT UNTIL IT REACHES MAIN The workflow checks out the DEFAULT BRANCH to run the scripts, so a change to agy-review.sh has no effect on any PR -- not even the PR that makes it. Swept from the canonical template at Local_Only-Projects/antigravity-pr-review/, which carries all of the above for future installs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
Owner
Author
Scope note: this PR now also carries the reviewer's archive fixPushed a second commit here rather than opening a separate PR, and the reason is What the new commit doesThe Antigravity reviewer posted a fresh comment each round and deleted the It now edits one comment per PR: newest round on top, earlier rounds folded Also here: Eight mutations, all caught. Selftest passes. Retitle or split if you'd rather keep this PR to its original subject — happy 🤖 Generated with Claude Code |
…ction Two mechanisms existed in exactly one of the five installs. Both belong in all of them, and the sweep that unified the comment-archive behaviour is the right moment to say so. A BACKEND OUTAGE POSTED AS A PASSING REVIEW When agy's upstream is down it prints an error rather than a review: Error: Eligibility check failed: UNAVAILABLE (code 503): The service is currently unavailable. That text is non-empty, so have_text() treated it as a valid review, POSTed it as the review comment, and the job exited 0 -- a green check for a review that never ran. Observed twice on SLAC PR #14, where the check passed in seven seconds with that string as its entire body. A control that cannot fail is worse than no control. The match is deliberately ANCHORED to the start of the capture rather than being a substring search, and it is bounded by size. A genuine review may quote a 503 or an UNAVAILABLE constant while reviewing retry logic, and aborting on that would be the false positive the OAuth guard's design notes warn about. A backend failure IS the whole capture and begins with `Error:`, so requiring the error on line one, in a capture short enough to contain nothing else, separates the two without a content heuristic. A backend error is transient, so it retries like empty output rather than aborting the way a lapsed session does -- but the capture is blanked so no later path can post it. The tally is a COUNTER, not a per-attempt flag: a boolean reset each attempt reflects only the last one, so a 503 on attempt 1 followed by empty output on attempt 3 would report the wrong cause. Both exit non-zero, so nothing unsafe -- but the log line is the only thing telling a human which outage they are looking at. MARKER-BASED EXTRACTION, AND WHY IT IS BETTER The selftest lifted the jq filter out of the reviewer by matching the declaration's own syntax: a sed range ending at the first line closing with a quote. A filter whose body ever ended a line that way would be SILENTLY TRUNCATED, and a truncated jq program can still compile and still return ids -- the exact silent-wrong-answer that file exists to prevent. Explicit `SELFTEST-EXTRACT` markers replace it. They also let a guard be several statements rather than one assignment, which is what makes the OAuth and service-error guards testable at all. Every marked block is now asserted to exist, to be valid shell, and to be sourceable, because a renamed marker would extract EMPTY -- and an empty guard sources fine and asserts nothing. WHAT THE MUTATIONS CHANGED Three of six came back NOT CAUGHT on the first pass, and two were real. The anchor could be deleted with every check still passing, because the fixture for "a review discussing a 503" put the error on line 3, where `head -n 1` already excluded it. A fixture whose FIRST line contains the error text mid-line -- which only `^` can reject -- now covers it. The persistent-outage abort was checked by grepping the script for its condition, which `if false && [ ... ]` still satisfies. That decision is now a named function, `backend_outage_should_fail`, called by the test rather than grepped for; three mutations of it are caught where the grep caught none. `have_text` moved inside the marked block so the block is self-contained -- the marker is a comment, so nothing about where the function is defined changed. The third, removing the `[ -s ]` empty-file check, is an EQUIVALENT mutant and is recorded as such rather than papered over with a test: an empty capture yields no grep match either way, so the check is defensive and its removal is unobservable. Superset verified rather than assumed: every non-comment line SLAC had before this sweep is either present in the template or is old delete machinery this design removes, plus a large-diff fallback the template supersedes -- SLAC's copy handled GitHub's 20,000-line limit only, the template's handles the 300-FILE limit too. All five installs now run one implementation. Selftest passes and actionlint is clean in each. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
…fault branch The reviewer workflow checks out the DEFAULT BRANCH to get its scripts -- that is deliberate and documented, so a fork's code never executes on the self-hosted runner. But for a `pull_request` event GitHub runs the workflow YAML itself from the PR BRANCH. The two halves therefore come from different refs, and a change spanning both breaks its own PR. The preceding commit added `scripts/_agy_comment_body.sh` and added it to the workflow's chmod; on RustyNES the job died with chmod: cannot access 'scripts/_agy_comment_body.sh': No such file or directory because the checkout was of `main`, which does not have the file yet. Every repo in this sweep would have hit the identical failure on its next run. The surprising half is the inversion. The default-branch rule is documented for the scripts -- a change to agy-review.sh has no effect until it merges -- and its corollary is that the workflow moves IMMEDIATELY while the scripts do not, which runs against the usual intuition that a PR is self-consistent. The workflow half now tolerates both script sets: the two required files are chmod'd unconditionally, anything added later only if present, with a trailing `true` so a false `[ -f ]` cannot fail the step under `bash -e`. A genuinely missing required file still fails loudly, because agy-review.sh sources it and dies -- the tolerance belongs in the workflow, not in the contract. actionlint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
`gh api -f body="$(cat "$body_file")"` passed the whole comment as a single
execve argument. At MAX_BODY_BYTES the archived comment approaches 60 KB against
a MAX_ARG_STRLEN of 128 KB on Linux -- close enough that raising the bound later
would start failing with E2BIG, and the failure would read as a GitHub API error
rather than a local limit.
It is now `jq -n --rawfile b "$body_file" '{body: $b}'` piped into
`gh api ... --input -`. Nothing traverses argv, and `--rawfile` makes the value a
JSON string by construction, so neither shell quoting nor `-F` type-coercion can
reinterpret a body that happens to look like a number or a boolean.
Selftest passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
`agy_drop_oldest_round` located the oldest archived round by matching `/^<details>$/` -- the tag itself. A review body legitimately contains `<details>` blocks: folded logs, collapsed code, another bot's summary, and the archived rounds are themselves nested `<details>`. So the cut could land INSIDE a round, leaving torn HTML and half a review in a comment nobody would think to check. It is now delimited by `AGY_ROUND_MARK`, an HTML comment the writer emits ahead of each round. Invisible when rendered, and it cannot occur by accident in prose the way a tag can. Same mechanism the archive boundaries already used -- markers for the outer boundaries and a naive regex for the inner ones was the kind of half-application that reads as consistent until someone tries it with real content. An archive written by the previous version has no round markers. With none present the function exits non-zero and NOTHING is dropped, so the edit fails on size -- recoverable, visible, repaired by the next round -- rather than the archive being silently mangled. Three checks and two mutations. The nested case is the one that matters: a round carrying its own `<details>` must survive the oldest being dropped, and restoring the tag-matching form fails it. Also asserted: the writer actually emits the sentinel, because otherwise every archive looks legacy-shaped, the trim silently never fires, and the comment grows until the edit fails. Selftest passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
RustyN64 enforces en-US spelling through `scripts/check_en_us.sh`, and two of my comment lines carried "behaviour". The check passes on that repo's `main` and failed on the PR, so this was mine rather than pre-existing. Fixed in the canonical template rather than in the one repo that complained. The five installs are byte-identical by design -- that is what makes the sync a copy instead of a re-derivation -- so a spelling fix applied locally would have made RustyN64 the odd one out and the next template sync would have reverted it. en-US is the safe common denominator: the repos that do not enforce it do not care, and the one that does now passes. Verified by running RustyN64's own checker against the template's scripts rather than by eye: it reported the same two lines before and reports none after. Not touched: `docs/provenance.md` in RustyN64 also trips the checker locally, but it is UNTRACKED there -- a stray working-tree file, not part of any PR, which is why CI counted two findings where a local run counts three. Deleting or editing another repo's untracked file to quiet a checker would be the wrong fix for a problem CI does not have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
`Cargo Deny Check` and `Dependency Audit` were failing on `main`, not merely on a branch, so the vulnerability had been shipping. RUSTSEC-2026-0257 -- `BROWSER` argument injection on Unix, reached transitively through `egui-winit` -> `webbrowser` 1.2.1. On Unix the affected versions substituted the caller's URL into the `BROWSER` template BEFORE tokenizing the result with `split_ascii_whitespace()`, so a URL that retains spaces could break out into additional browser arguments. Upstream reproduced it against Chromium with `--remote-debugging-port`, exposing a local DevTools endpoint, and `--proxy-server`, redirecting traffic through an attacker-controlled proxy. Fixed in 1.2.2 by tokenizing the template first; this lands 1.2.4. EVERY DEPENDENCY, NOT JUST THE VULNERABLE ONE 141 packages moved to their latest semver-compatible versions. A targeted patch of the one crate would have cleared the gate while leaving the rest of the graph wherever it happened to be; a current graph is the state a security gate can meaningfully assert against, and it is cheaper to verify once than to re-verify per advisory. Verified rather than assumed. `cargo deny check` reports advisories ok, bans ok, licenses ok, sources ok. The workspace is green at 66 suites / 897 tests / 0 failures, with `cargo fmt --all --check`, `cargo clippy --workspace --all-targets -- -D warnings` and a full `cargo build --workspace` all passing. That test run is the point of the exercise: a green `cargo deny` says nothing about whether 141 package updates changed emulator behaviour, and the suite is the only thing that can answer it. TWO IGNORES RETIRED ON THEIR OWN STATED CONDITION `deny.toml` ignored the `quick-xml 0.39.4` pair (RUSTSEC-2026-0194 and RUSTSEC-2026-0195) with the note that no upgrade path existed -- the fixes live in quick-xml >= 0.40, `wayland-scanner` 0.31.10 pinned ^0.39, and the entry said to revisit on the next wayland-scanner / smithay-client-toolkit release. That release arrived in this update: wayland-scanner 0.31.10 -> 0.31.11 pulls quick-xml 0.39.4 -> 0.41.0. `cargo deny` flagged both ignores as matching no crate, which is the signal that an ignore has outlived its reason, and the check stays green with them removed. The `ttf-parser` ignore (RUSTSEC-2026-0192) is KEPT and still matches: an informational "unmaintained" advisory, not a vulnerability, on a transitive dep of winit's Wayland decoration stack with no upgrade available. NOT TOUCHED `## [1.21.0] "Touchstone"` carries two sibling `### Changed` headings, which trips MD024 under this repo's `siblings_only` config. It predates this change by many releases -- confirmed against HEAD -- and quietly folding an unrelated lint fix into a security bump would make both harder to review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
Antigravity review (Gemini via Ultra)This PR updates the CI reviewer script to fall back to a local git diff when GitHub's API 300-file limit is reached, retrieves the merge base via the API for shallow clones, edits the existing PR review comment in-place to archive older rounds instead of deleting them, and bumps workspace dependencies to their latest compatible versions. Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Synced from RustyNES, where this was found and fixed
(doublegate/RustyNES#374).
Byte-identical to the version landing there.
The bug
On a wide PR the Antigravity reviewer fails with
gh pr diff failed:and anempty reason, which looks exactly like a lapsed OAuth session on the self-hosted
runner. It isn't. GitHub refuses an oversized diff two ways, both HTTP 406,
with different wording:
The local-diff fallback was gated on only the first:
grep -qi 'diff exceeded the maximum number of lines'So a wide-but-shallow PR — hundreds of files, well under the line limit,
which is exactly what a bulk regeneration of test baselines produces — took the
elsebranch and exited 1. That is the one outcome the fallback exists toprevent; the comment directly above it reads "a large PR is exactly the one
worth reviewing."
Verified, not assumed
On RustyNES the path behind the fix was exercised as the script does it — fetch
the PR head and base into the private
refs/agy/*namespace, diff against themerge base. A 487-file PR produced 12,002 lines / 586,010 bytes, well inside
the 5 MB
MAX_DIFF_BYTEScap, so the review proceeds normally once the grepmatches.
Includes both review findings from the upstream PR
number — it would mean nothing in this repo.
Hardcoding "20,000-line" for a file-count refusal is the same misleading
triage signal that produced the wrong diagnosis in the first place.
Note
This cannot take effect on the PR that carries it. The workflow checks out the
default branch for the reviewer scripts, by design, so a PR cannot rewrite
its own reviewer.